-
Notifications
You must be signed in to change notification settings - Fork 86
[GuideLLM Refactor] Scenarios reenablement #362
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…e-draft [GuideLLM Refactor] Scenarios reenablement #362
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This appears to break the non-scenario pathway. And I'm running into issues running the built-in scenario.
Regarding the issues with the built-in profile:
The built-in scenario rag
appears to use the old name for the profile
, rate_type
. The result of this is the command line still requesting that I specify the profile type despite it being included in the profile.
There is also an error trying to use the rag
profile:
File "/Users/joconnel/Documents/projects/ai/guidellm/src/guidellm/dataset/creator.py", line 94, in create
dataset = cls.handle_create(
data, data_args, processor, processor_args, random_seed
)
File "/Users/joconnel/Documents/projects/ai/guidellm/src/guidellm/dataset/in_memory.py", line 42, in handle_create
data_dict = cls.format_data_dict(data)
File "/Users/joconnel/Documents/projects/ai/guidellm/src/guidellm/dataset/in_memory.py", line 64, in format_data_dict
raise TypeError(
...<2 lines>...
)
TypeError: Unsupported data format. Expected Dict[str, Iterable[Any]], got <class 'dict'>
Are the built in scenarios up to date?
Regarding the issue with the non-scenario pathway, it's sending a list of integers to the profiles that need an integer, causing a validation failure:
guidellm benchmark run --target http://localhost:8000 --data "prompt_tokens=256,output_tokens=128" --rate-type constant --rate 2 --max-seconds 10
TypeError: Unable to apply constraint 'gt' to supplied value [2.0]
It's possible that some of this is user error, but if that's the case the input validation is not doing a good job.
ea26181
to
fb5c153
Compare
It turns out the built-in scenarios have been broken for a long while. I don't love how this feature works at the moment and want to rethink it in the next update. For now this provides basic reenablement in the refactor. |
fb5c153
to
d57fe3d
Compare
1e47d98
to
68e9f55
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. With the fix I pushed it works for me, though I haven't had the time to let a full run complete.
I have one comment, but I won't block on that. I am also willing to do that work for you if you like the idea.
Replace scenario entrypoint with a decorator Forward-port get_default and from_file to Scenario Apply scenario args as an update to kwargs Readd scenario support to CLI Signed-off-by: Samuel Monson <[email protected]>
Signed-off-by: Samuel Monson <[email protected]>
Signed-off-by: Samuel Monson <[email protected]>
Signed-off-by: Samuel Monson <[email protected]>
Signed-off-by: Jared O'Connell <[email protected]>
68e9f55
to
03f9085
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the scenario system in GuideLLM to enable scenarios functionality. The changes transition from a nested JSON structure to a flat string-based data format and introduce comprehensive scenario support across the codebase.
- Simplifies scenario JSON configuration files to use flat string data format instead of nested objects
- Adds comprehensive scenario support with validation, file loading, and decorator functionality
- Integrates scenarios into the CLI with builtin scenario selection and file-based configuration
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
src/guidellm/benchmark/scenarios/rag.json | Converts nested data structure to flat string format |
src/guidellm/benchmark/scenarios/chat.json | Converts nested data structure to flat string format |
src/guidellm/benchmark/scenario.py | Adds scenario loading, validation, and decorator functionality |
src/guidellm/benchmark/profile.py | Updates type annotations and removes redundant type checks |
src/guidellm/benchmark/entrypoints.py | Adds scenarios decorator to benchmark function |
src/guidellm/benchmark/benchmarker.py | Fixes constraints parameter handling |
src/guidellm/benchmark/init.py | Exports new scenario-related symbols |
src/guidellm/main.py | Integrates scenario support into CLI with validation |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Signed-off-by: Jared O'Connell <[email protected]>
bb74a55
to
2c6637d
Compare
Summary
Details
Test Plan
Related Issues
Use of AI
## WRITTEN BY AI ##
)